Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core] Modern visitor creation #5126

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

alxbilger
Copy link
Contributor

@alxbilger alxbilger commented Nov 15, 2024

This PR makes several suggestions, and should not be considered as is for a merge. A choice on the implementations must be made.

  • Implementation of the true visitor pattern (void accept(TopDownVisitor& visitor)) https://refactoring.guru/design-patterns/visitor/cpp/example. It is a new implementation of the visitors, independent from the old visitors.
  • Modern and flexible creation of new visitor at the call site using composable callables with the pipe operator. Some old visitors can be replaced by the new mechanism, and the class can be removed.
  • Modern and flexible creation of old visitor at the call site. It uses the function makeMechanicalVisitor.
  • Some examples of use, and unit tests. In this PR, the file MechanicalOperations uses a mix of old fashion visitors, and new visitors. The file must be reviewed as an example of usage.

Example of the new visitors:

ctx->accept(objectmodel::topDownVisitor
    | [this, &df](core::behavior::BaseForceField* force)
    {
        force->addMBKdx(&mparams, df);
    },
    
    objectmodel::bottomUpVisitor
    | [this, &df, accumulate](core::BaseMapping* mapping)
    {
        if (accumulate)
        {
            mapping->applyJT(&mparams, df, df);
            if( mparams.kFactor() != 0_sreal )
            {
                mapping->applyDJT(&mparams, df, df);
            }
        }
    });

Example of the old visitors:

executeVisitor( makeMechanicalVisitor(&mparams,
    TopDownMassCallable([this, &result](simulation::Node* /*node*/, core::behavior::BaseMass* mass)
    {
        if( mass->m_separateGravity.getValue() )
        {
            mass->addGravityToV(&mparams, result);
        }
        return Visitor::RESULT_CONTINUE;
    })
) );

Pros:

  • The pipe operator is consistant with the design of the STL range library.
  • No more back-and-forth between the call site and the visitor file. The intention can be read clearly at the call site.
  • Dozens of visitor files can be removed
  • The new visitor mechanism is much clearer and closer to the visitor design pattern.

Cons:

  • A bit of template meta-programming
  • Some ugly macros
  • The call-stack is a much clearer with the new visitors but still difficult for a novice
  • More code at the call site (but it's so much clearer so it's not a con😄 )

TODO with the new visitors:

  • Fix ambiguity between BaseForceField and BaseInteractionForceField
  • More tests. In particular, with diamond scene graph or multiple parents
  • Consistency with BaseMechanicalVisitor and old visitors
  • Support detection of mapped states
  • constexprify the visitors

The question is: which implementation of the visitors do we keep?

Future work: Implement this kind of "visitor" for the mapping graph


By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).


Reviewers will merge this pull-request only if

  • it builds with SUCCESS for all platforms on the CI.
  • it does not generate new warnings.
  • it does not generate new unit test failures.
  • it does not generate new scene test failures.
  • it does not break API compatibility.
  • it is more than 1 week old (or has fast-merge label).

@alxbilger alxbilger added enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request pr: dev meeting topic PR to be discussed in sofa-dev meeting labels Nov 15, 2024
@alxbilger
Copy link
Contributor Author

The unit tests fail because lambdas on BaseForceField are not called on BaseInteractionForceField, which is the case in old visitors.

@fredroy
Copy link
Contributor

fredroy commented Nov 18, 2024

[ci-build][with-all-tests]

BottomUpVisitor() = default;
};

inline TopDownVisitor topDownVisitor {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using the same name with only no capital letter at the beginning is misleading. Given the fact that this is kind of equivalent to a static object, why not add a suffix ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no guidelines in SOFA relative to the naming of global variables (I already saw a convention with a g_ prefix). But I agree with you the names of the type and the variable are too similar. The STL has the same design for execution policy, and the variable names are truncated to differentiate with the type names. But here, I don't like it so much. Let's discuss that in the dev meeting.
Keep in mind that this is a detail of one of the implementation of the visitors. We still need to decide which one of both implementations we keep. That's the main question of this PR.

@hugtalbot
Copy link
Contributor

@sofa-framework/reviewers your opinion is needed.

While working on the visitor definition, Alex proposes two alternative methods: the creation of classical SOFA visitors or a new implementation of the original visitor design pattern. It is a known pattern, slightly more verbous but the implementation follows modern C++ format (accept combined with lambda). A second version of these modern visitors is presented (see MechanicalOperations.cpp) with a function e.g. makeMechanicalVisitor. This might lead to code duplication.
Objective is to improve the visitor readability at first sight. This work is done with the objective to apply visitors on a "mapping-based graph".

We thought @JeremieA you could have an interesting point of view about this.

@hugtalbot hugtalbot marked this pull request as draft November 20, 2024 08:55
@hugtalbot hugtalbot removed the pr: dev meeting topic PR to be discussed in sofa-dev meeting label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement About a possible enhancement pr: status to review To notify reviewers to review this pull-request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants